accept text/plain type by default for prometheus client scraping #24622

newly12 · 2021-03-18T06:47:55Z

What does this PR do?

accept text/plain type by default for prometheus client scraping

Why is it important?

prometheus java client 0.10.0 introduced breaking changes that when application/openmetrics-text is seen in accept list, new types(i.e. info) will be exposed and they are not supported by current metrics parser

Checklist

~~My code follows the style guidelines of this project~~
~~I have commented my code, particularly in hard-to-understand areas~~
~~I have made corresponding changes to the documentation~~
~~I have made corresponding change to the default configuration files~~
~~I have added tests that prove my fix is effective or that my feature works~~
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

[ ]

How to test this PR locally

start a java app instrumented with prometheus client version 0.10.0 and then start metricbeat to scrape it

Related issues

Closes new openmetrics types are not supported for prometheus module #24554

Use cases

Screenshots

Logs

elasticmachine · 2021-03-18T06:53:01Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: jsoriano commented: /test
Start Time: 2021-03-24T12:50:11.264+0000
Duration: 63 min 23 sec
Commit: 307060b

Test stats 🧪

Test	Results
Failed	0
Passed	8899
Skipped	2385
Total	11284

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	8899
Skipped	2385
Total	11284

elasticmachine · 2021-03-18T08:27:15Z

Pinging @elastic/integrations (Team:Integrations)

jsoriano

Thanks for reporting this!

they are not supported by current metrics parser

Does it produce failures in Metricbeat?

jsoriano · 2021-03-18T16:09:32Z

metricbeat/helper/prometheus/prometheus.go

+const (
+	promTextHeader    = `text/plain;version=0.0.4;q=0.5,*/*;q=0.1`
+	openmetricsHeader = `application/openmetrics-text; version=0.0.1,text/plain;version=0.0.4;q=0.5,*/*;q=0.1`
+	acceptHeader      = promTextHeader


Metricbeat has two collectors, one for prometheus, and another one for openmetrics, they are mostly the same so far, but I think it would make sense if each collector configured the default headers of their choice.

Thanks for the comments. yeah but I think for application/openmetrics-text the parser would only work for 'previous' openmetrics format, not the new one, so sending Accept: application/openmetrics-text could give target endpoints the chance to respond with new format, while at this moment, text/plain prometheus format should still be compatible from their perspective.

This PR is more like a workaround as being mentioned in the issue, the solution would still be updating the parser as suggested by @exekias .

I wonder why application/openmetrics-text; version=0.0.1 includes the new openmetrics types 🤔 . Shouldn't that change be applied with a new version so as to be backwards compatible?

good point. I think somehow that is not happening and hence breaking things.

jsoriano · 2021-03-18T16:13:37Z

cc @ChrsMark @exekias

jsoriano · 2021-03-22T12:53:51Z

/test

ChrsMark · 2021-03-22T14:29:18Z

Looking at #17291 this header was added by Blake to work in general for all cases since these are the headers used by prometheus upstream too.

In the issue that Blake tried to fix (#16870) I found a config that helps the user and might be helpful in this case too: #16870 (comment).

@newly12 do you think sth like the following could work in your case?

- module: prometheus
  period: 10s
  hosts: ["localhost:80"]
  metrics_path: /platform/prometheus
  headers:
    accept: "`text/plain;version=0.0.4;q=0.5,*/*;q=0.1"

newly12 · 2021-03-23T01:46:42Z

thanks @ChrsMark , it should work by setting overridden header in config. concern is that this needs to be removed after the long term fix done in metricbeat and before prom text compatible removed(if they will) in various sdk. I would assume long term fix requires parser update(i.e. move to textparse?) and maybe effort for new types mapping, which sounds like may not happen in a short period of time..

ChrsMark · 2021-03-23T10:16:37Z

@newly12 I will create a separate issue to track the investigation of the updates that are required (I will check textparse thnaks!). Do you think it's ok to close this one (along with the original issue) if overriding the header works for you? Then we can continue the chat at the follow up issue (#24707).

vjsamuel · 2021-03-24T02:31:33Z

@ChrsMark this is an issue everytime someone tries to report a metric of type info. especially when we use hints based autodiscover we cant keep setting headers for each use case. given that we dont natively support open metrics scraping at this time, how can we set that as the default header? we should not use it, unless we move to textparse which is the only native openmetrics parser at the moment. expfmt is more or less deprecated.

ChrsMark · 2021-03-24T08:13:00Z

Makes sense @vjsamuel , thanks for reporting this. I think we can proceed with this PR and merge it since as described at #24707 (comment) adding application/openmetrics-text does not change anything in the process.

@jsoriano @exekias what do you think about merging this one?

jsoriano · 2021-03-24T08:54:35Z

Sounds good to me.

exekias · 2021-03-24T08:56:46Z

I'm ok with patching this way for now. One question on my end, where is openmetricsHeader used right now?

ChrsMark · 2021-03-24T08:59:34Z

I'm ok with patching this way for now. One question on my end, where is openmetricsHeader used right now?

Nowhere, we can remove it.

ChrsMark

Let's proceed with merging this one after we cleanup the unwanted headers.

metricbeat/helper/prometheus/prometheus.go

newly12 · 2021-03-24T11:48:00Z

Thanks all! I've removed unnecessary openmetricsHeader.

jsoriano · 2021-03-24T12:49:49Z

/test

ChrsMark

lgtm, thanks @newly12 for your effort here!

…stic#24622) (cherry picked from commit e2257b9)

…etheus client scraping (#24731)

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Mar 18, 2021

andresrc added the Team:Integrations Label for the Integrations team label Mar 18, 2021

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Mar 18, 2021

jsoriano reviewed Mar 18, 2021

View reviewed changes

newly12 added 2 commits March 22, 2021 10:03

accept text/plain type by default for prometheus client scraping

2abaae1

update CHANGELOG.next.asciidoc

3512e9d

newly12 force-pushed the prometheus_http_header branch from 16f1523 to 3512e9d Compare March 22, 2021 02:11

jsoriano added the review label Mar 22, 2021

ChrsMark requested changes Mar 24, 2021

View reviewed changes

metricbeat/helper/prometheus/prometheus.go Outdated Show resolved Hide resolved

update header

307060b

newly12 force-pushed the prometheus_http_header branch from b507e28 to 307060b Compare March 24, 2021 11:46

jsoriano requested a review from ChrsMark March 24, 2021 12:50

ChrsMark approved these changes Mar 24, 2021

View reviewed changes

ChrsMark merged commit e2257b9 into elastic:master Mar 24, 2021

ChrsMark mentioned this pull request Mar 24, 2021

Cherry-pick #24622 to 7.x: accept text/plain type by default for prometheus client scraping #24731

Merged

6 tasks

ChrsMark pushed a commit to ChrsMark/beats that referenced this pull request Mar 24, 2021

Accept text/plain type by default for prometheus client scraping (ela…

b72abc6

…stic#24622) (cherry picked from commit e2257b9)

ChrsMark added the v7.13.0 label Mar 24, 2021

newly12 deleted the prometheus_http_header branch March 24, 2021 15:49

ChrsMark added a commit that referenced this pull request Mar 24, 2021

Cherry-pick #24622 to 7.x: accept text/plain type by default for prom…

29c38b4

…etheus client scraping (#24731)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

accept text/plain type by default for prometheus client scraping #24622

accept text/plain type by default for prometheus client scraping #24622

newly12 commented Mar 18, 2021 •

edited

Loading

elasticmachine commented Mar 18, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

elasticmachine commented Mar 18, 2021

jsoriano left a comment

jsoriano Mar 18, 2021

newly12 Mar 19, 2021

ChrsMark Mar 22, 2021

newly12 Mar 23, 2021

jsoriano commented Mar 18, 2021

jsoriano commented Mar 22, 2021

ChrsMark commented Mar 22, 2021

newly12 commented Mar 23, 2021

ChrsMark commented Mar 23, 2021 •

edited

Loading

vjsamuel commented Mar 24, 2021

ChrsMark commented Mar 24, 2021

jsoriano commented Mar 24, 2021

exekias commented Mar 24, 2021

ChrsMark commented Mar 24, 2021

ChrsMark left a comment

newly12 commented Mar 24, 2021

jsoriano commented Mar 24, 2021

ChrsMark left a comment

accept text/plain type by default for prometheus client scraping #24622

accept text/plain type by default for prometheus client scraping #24622

Conversation

newly12 commented Mar 18, 2021 • edited Loading

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

elasticmachine commented Mar 18, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

elasticmachine commented Mar 18, 2021

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano Mar 18, 2021

Choose a reason for hiding this comment

newly12 Mar 19, 2021

Choose a reason for hiding this comment

ChrsMark Mar 22, 2021

Choose a reason for hiding this comment

newly12 Mar 23, 2021

Choose a reason for hiding this comment

jsoriano commented Mar 18, 2021

jsoriano commented Mar 22, 2021

ChrsMark commented Mar 22, 2021

newly12 commented Mar 23, 2021

ChrsMark commented Mar 23, 2021 • edited Loading

vjsamuel commented Mar 24, 2021

ChrsMark commented Mar 24, 2021

jsoriano commented Mar 24, 2021

exekias commented Mar 24, 2021

ChrsMark commented Mar 24, 2021

ChrsMark left a comment

Choose a reason for hiding this comment

newly12 commented Mar 24, 2021

jsoriano commented Mar 24, 2021

ChrsMark left a comment

Choose a reason for hiding this comment

newly12 commented Mar 18, 2021 •

edited

Loading

elasticmachine commented Mar 18, 2021 •

edited by jenkins-beats-ci bot

Loading

ChrsMark commented Mar 23, 2021 •

edited

Loading